-
Notifications
You must be signed in to change notification settings - Fork 44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Errors for promise issues in eslint... #8796
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Not sure we want to remove eslint warning recommendations
- We should fix the promise issues instead of ignoring them with the comments cause then we won't ever get around to fixing them and assume the comments are correct.
I wanted to clarify the intent of this PR. I only want to enable lint-diff and then grandfather anything necessary. Fixing each issue would mean potentially introducing bugs in this PR plus fixing too many issues at once. Also, if I fix them, I could fix them wrong. So every change might increase PR latency. Just grandfathering them (along with probably 30 there ones via the lint-diff config) was the strategy I'm going for. I'm totally fine if we want to come back and resolve these after the fact but we also have a lot of other promise violations int he code that are grandfathered - we should probably fix those too. I also put back in the lint-branch-warnings for now until I have to the time to make sure lint-diff works with all our other rules. Without this change we were potentially adding new thread bugs and since these are often pretty serious errors I wanted to fix this quickly. |
Link to Issue
Closes: #8793
Description of Changes
We will still have to improve our lint rules but at least hard errors are being fixed.
Also, important note. I REMOVED lint-branch-warnings. We don't need it anymore and there are no advantages since we're running lint-diff. Also, lint-branch-warnings will cause false positives in some situations so it's actually problematic to our setup. Basically, if you touch a file, it's seen as 'changed' so you will start getting errors.
"How We Fixed It"
Test Plan
Deployment Plan
Other Considerations